fix: translate SQL wildcards in SIMILAR TO patterns (#22263) - #23188
fix: translate SQL wildcards in SIMILAR TO patterns (#22263)#23188oc7o wants to merge 15 commits into
Conversation
`SIMILAR TO` previously passed the pattern straight to Arrow's regex
engine, so SQL wildcards were never translated and matches were
unanchored:
SELECT 'abc' SIMILAR TO 'a%'; -- returned false
SELECT 'x' SIMILAR TO '_'; -- returned false
Translate `%` to `.*` and `_` to `.`, then wrap the pattern in
`^(?:...)$` so the regex matches the entire string. Other regex
metacharacters (`|`, `(`, `)`, `*`, `+`, `?`) pass through unchanged,
matching `SIMILAR TO`'s superset-of-regex semantics.
The translation only fires for literal `Utf8`, `LargeUtf8`, and
`Utf8View` patterns. Non-literal patterns return a `not_impl_err!` —
silently wrong results are worse than an honest error, and this mirrors
how DataFusion already handles the unsupported `ESCAPE` clause. NULL
patterns pass through unchanged.
Existing tests in `binary.rs` were relying on the bug by passing raw
regex strings as `SIMILAR TO` patterns; they have been rewritten to use
SQL wildcard syntax, and new cases cover `%`, `_`, full-string
anchoring, and regex-metacharacter passthrough. End-to-end coverage
added in `strings.slt`.
|
@huaxingao @viirya @wesm Could one of you trigger CI for me please? Thanks! |
|
@oc7o Triggered. CI is running now. |
There was a problem hiding this comment.
@oc7o
Thanks for working on this. The direction looks good, but I think there are still a couple of correctness issues in the SIMILAR TO translation that should be addressed before this can be merged. I also have one small test coverage suggestion.
|
@kosiew I gotta thank you for the effort you put into this. It was really with the eye for detail andI really learned a lot! 😊 I think with how it now is we're a bit closer to the standard SQL implementation. Maybe (when this PR is ready 🤞) escaping could be a good follow up topic for me. Since we currently still treat I'm curios for any responses! |
There was a problem hiding this comment.
@oc7o
Thanks for the updates here. The literal-pattern handling looks much better now: the regex-only literal characters are escaped, % and _ handle newlines, and the added Rust and SLT coverage is helpful.
I do still see one regression around dynamic pattern expressions. SIMILAR TO previously allowed the right-hand side to be a normal expression, but this PR now rejects non-literal patterns during planning. Could you please address that before this lands?
+ regression tests
Changed the runtime backstop errors from internal_err! to exec_err!
|
Thx again for the review. I learn so much new stuff when working on this! I now added support for dynamic values on the RHS alongside some bug fixes like for the One issue still is that datafusion-proto can't serialize Also a decision I consciously made was disregarding strings inside 🐙 |
Since this PR introduces Recommendation: if the goal is to keep this PR small, limit this PR's SQL-correct translation to literal patterns and preserve the previous dynamic-pattern representation for now. In practice, that means translating only let translated_pattern = match pattern.downcast_ref::<crate::expressions::Literal>() {
Some(literal) => Arc::new(crate::expressions::Literal::new(translate_scalar(
literal.value(),
)?)) as Arc<dyn PhysicalExpr>,
None => pattern,
};Then in a follow up PR, add the SqlSimilarToPattern AND proto support. |
|
Looks like the PR blew up a bit 🥲 but I wanna do this right, and I think the relevant things are now well covered. About 750 of the changed lines are tests and 300 hand-written source (plus the generated proto code). Proto support: I went with your first option rather than narrowing the PR. Type coercion: while testing the above I found an issue with the plan time type check. The plan-time check now accepts
|
|
@oc7o |
The coercion tests added in apache#23704 used `(auth|login)` as a non-scalar pattern and expected it to match `user auth failed`. That only held because `SIMILAR TO` matched unanchored, which is the bug this branch fixes: the pattern must now match the entire string. Wrap the alternation in `%` wildcards so the three cases still return true and keep testing what they were written for -- that Utf8View, LargeUtf8 and Dictionary values are coerced to a common type with the pattern and reach the regex kernel.
SIMILAR TOpreviously passed the pattern straight to Arrow's regex engine, so SQL wildcards were never translated and matches were unanchored:Translate
%to(?s:.*)and_to(?s:.)(dot-all so they match newlines), then wrap the pattern in^(?:...)$so the regex matches the entire string..,^,$, and\are escaped as SQL literals. The POSIX metacharacters thatSIMILAR TOdefines (| * + ? ( ) { } [ ]) pass through to the regex unchanged.Which issue does this PR close?
SIMILAR TOshould treat%as a wildcard #22263.Rationale for this change
SIMILAR TOis a SQL standard operator with well-defined wildcard semantics (%= any sequence,_= single character, full-string match). DataFusion's previous behavior silently produced wrong results for the most basic patterns, which is a correctness bug for anyone porting queries from Postgres or other SQL engines.Supporting non-literal patterns requires a new physical expression, so that expression also needs protobuf support — otherwise serialized physical plans containing a dynamic
SIMILAR TOwould regress for distributed engines built ondatafusion-proto.Accepting
LargeUtf8andUtf8Viewpatterns in turn requires type coercion.Expr::SimilarTowas previously in the analyzer's no-op list, so nothing reconciled the value type with the pattern type. Since the regex kernel dispatches on the left-hand type and then downcasts the pattern to that same array type, any mismatch aborted the query with afailed to downcast arraypanic.What changes are included in this PR?
Pattern translation
sql_similar_to_regexhelper that translates%/_and anchors the pattern with^(?:...)$. It tracks bracket state so^inside[...]is treated as bracket negation, not as a literal, and so SQL wildcards lose their special meaning inside a bracket expression.SqlSimilarToPatternphysical expression indatafusion/physical-expr/src/expressions/similar_to_pattern.rsthat applies the translation at runtime for non-literal patterns.similar_to()now translates literalUtf8/LargeUtf8/Utf8Viewpatterns at planning time and wraps non-literal patterns inSqlSimilarToPatternfor runtime translation.NULLinstead of crashing with an internal error. The translation preserves the pattern's string variant, including for NULL, so the pattern type always matches the value type.datafusion/sql/src/expr/mod.rsthat rejects non-string patterns with a cleanplan_err!.SqlSimilarToPatternare reported asexec_err!rather thaninternal_err!.Type coercion
Expr::SimilarToarm todatafusion/optimizer/src/analyzer/type_coercion.rsand removedExpr::SimilarTofrom the no-op list. It usesregex_coercion, the same coercionOperator::RegexMatchalready uses, since that is whatSIMILAR TOlowers to. Mismatched string types are now cast to a common type instead of reaching the kernel and panicking.Protobuf support
PhysicalSqlSimilarToPatternNodemessage and wired it intoPhysicalExprNode.expr_typeas field 24.try_to_proto/try_from_protoforSqlSimilarToPatternand added the decode arm indatafusion/proto/src/physical_plan/from_proto.rs.prost.rs/pbjson.rsviadatafusion/proto-models/regen.sh.Are these changes tested?
Yes.
Pattern semantics (
datafusion/physical-expr/src/expressions/binary.rs):test_similar_to_sql_literal_metacharsconfirms that.,^,$, and\are treated as SQL literals, not as regex operators.test_similar_to_posix_metacharsconfirms that|, *, +, ?, (, ), {, }, [, ], [^...], and[a-z]behave asSIMILAR TOmetacharacters.test_similar_to_wildcards_match_newlinesconfirms that%and_match newlines.test_similar_tocovers basic%/_semantics, full-string anchoring, and case sensitivity.test_similar_to_dynamic_patterncovers column-based patterns.test_similar_to_null_patternandtest_similar_to_non_literal_pattern_errorscover the NULL pattern and non-string literal pattern paths.Translation and coercion:
test_translate_scalar/test_translate_arraycover the translation itself, including that each string variant (and its NULL) is preserved.similar_to_for_type_coercionintype_coercion.rscovers matching types, mismatched string types, a NULL pattern cast to the value's type, and the no-common-type error.Protobuf (
similar_to_pattern.rsproto_tests, mirroring the existingLikeExprtests):SqlSimilarToPatternnode, rejection of a missingexprfield, and error propagation on both the encode and decode paths.roundtrip_sql_similar_to_patternindatafusion/proto/tests/cases/roundtrip_physical_plan.rscovers the full plan roundtrip.End-to-end (
datafusion/sqllogictest/test_files/strings.slt):SIMILAR TO 'p[12].*'/NOT SIMILAR TO 'p[12].*'cases (which only worked because of the bug) were replaced with equivalent cases using standard wildcard syntax.SELECT 'a' SIMILAR TO NULL, rejection ofSELECT 'a' SIMILAR TO 1, mixedUtf8/LargeUtf8/Utf8Viewoperands, and dictionary-encoded values with both literal and dynamic patterns.Are there any user-facing changes?
Yes:
SIMILAR TOnow produces correct results for queries that were previously returning wrong answers..,^, or$as regex metacharacters) now follow standard SQLSIMILAR TOsemantics.SIMILAR TOPOSIX metacharacters (| * + ? ( ) { } [ ]) now work as expected.%and_wildcards now work.not_impl_err!, and physical plans containing them can be serialized withdatafusion-proto.LargeUtf8andUtf8Viewpatterns are accepted, and mixing string types between the value and the pattern is coerced rather than failing.SELECT ... SIMILAR TO NULLnow returnsNULLinstead of crashing.SELECT ... SIMILAR TO <non-string>now returns a clean plan error instead of an internal error.\is still treated as a literal backslash rather than as the SQL default escape character, and an explicitESCAPEclause is still rejected. Escape support is left as a follow-up.